Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(tests): Add integration test framework, goldens for 4 APIs [gapic-generator-python] #905

Merged
merged 8 commits into from
May 27, 2021

Conversation

miraleung
Copy link
Contributor

@miraleung miraleung commented May 21, 2021

The relevant files to review are those ending in .bzl. Any .py files in this PR are generated goldens only.

@miraleung miraleung requested a review from a team as a code owner May 21, 2021 22:31
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label May 21, 2021
@codecov
Copy link

codecov bot commented May 21, 2021

Codecov Report

Merging #905 (0c9c1a3) into master (7c185e8) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #905   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           26        27    +1     
  Lines         1608      1697   +89     
  Branches       328       347   +19     
=========================================
+ Hits          1608      1697   +89     
Impacted Files Coverage Δ
gapic/samplegen_utils/types.py 100.00% <ø> (ø)
gapic/samplegen_utils/utils.py 100.00% <ø> (ø)
gapic/utils/case.py 100.00% <ø> (ø)
gapic/utils/reserved_names.py 100.00% <ø> (ø)
gapic/generator/generator.py 100.00% <100.00%> (ø)
gapic/samplegen/samplegen.py 100.00% <100.00%> (ø)
gapic/schema/api.py 100.00% <100.00%> (ø)
gapic/schema/metadata.py 100.00% <100.00%> (ø)
gapic/schema/wrappers.py 100.00% <100.00%> (ø)
gapic/utils/__init__.py 100.00% <100.00%> (ø)
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 592ec06...0c9c1a3. Read the comment docs.

@miraleung miraleung requested a review from a team as a code owner May 21, 2021 22:46
Copy link
Contributor

@busunkim96 busunkim96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @miraleung! My Bazel knowledge is lacking so I'll let other folks review.

.github/workflows/tests.yaml Show resolved Hide resolved
Copy link
Contributor

@vam-google vam-google left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good to me, but a few general questions/concerns:

  1. It looks like a massive update, do we really need that many golden files? Can we pick like one API instead (with most feature coverage in it)? This is simply to reduce the number of goldens to maintain.

  2. Most importantly this will change the development workflow - with every new feature or substantial modification, the golden tests most probably will have to be updated. Also, unfortunately, bazel is and alien technology for most of the Python developers (including gapic-generator-python developers), and making them to deal with unexpected nasty failures and necessity to run/update the tests in the bazel way can make them really unhappy. Basically I was under impression that if we do goldens for gapic-generator-python it must be done in the python-idiomatic way. Comparing to gapic-generato-java - the generator itself is build via bazel, so using bazel for goldens infra makes perfect sense. For python the things are a bit different. I'm afraid if we introduce the goldens in this current form, relatively soon the maintainers of the repo will remove them as very painful/non-idiomatic to maintain.

  3. Even if we go with the goldens-via-bazel infra, is there some documentation speaking about how the development workflow is going to be changed by this, what developers need to run before posting the PR to make sure that the stuff passes and what to do if the goldens need to be updated (sorry if it is already there - i did try to all potential readme/.rst files in this PR but did not find documentation like that in those files).

@miraleung
Copy link
Contributor Author

Comments inline:

  1. In the Java generator, there were often distinct edge cases between different APIs - there isn't one that provides the most coverage. Maintaining golden files should be easy, since one only needs to run the Bazel target to update them all (and presumably devs should check the diffs manually to validate correctness). This set is meant to be a starter, will leave it to @busunkim96 and @software-dov to decide how many or which APIs we should actually be used.
  2. Since the microgenerator has Bazel rules, using Bazel in this repo seems to align with that. It also seems like we are missing a test path that validates they continue to work as expected, which is an added benefit of these integration tests.
  3. Updated DEVELOPMENT.md. The Git hooks in feat(dev): Add Git pre-commit hooks [gapic-generator-python] #908 will ensure all the pre-commit checks will run automatically on each commit.

Copy link
Contributor

@vam-google vam-google left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but please, wait for Bu Sun an Dov to approve (and also, still hope that we can reduce the number of golden files here).

Copy link
Contributor

@busunkim96 busunkim96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks Mira!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants